Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Cadence 1.0] Optionally check atree storage health before migration #5591

Merged

Conversation

turbolent
Copy link
Member

Work towards onflow/cadence#3192

The atree storage health check after the migration might error because the input was already unhealthy.

Optionally check the health before the migration, and do not error if the output is unhealthy when the input was already unhealthy.

@turbolent turbolent requested review from a team March 26, 2024 22:21
@turbolent turbolent self-assigned this Mar 26, 2024
@codecov-commenter
Copy link

codecov-commenter commented Mar 26, 2024

Codecov Report

Attention: Patch coverage is 65.04854% with 36 lines in your changes are missing coverage. Please review.

Project coverage is 57.30%. Comparing base (1b7ecca) to head (902b189).

Files Patch % Lines
...util/ledger/migrations/cadence_values_migration.go 37.25% 30 Missing and 2 partials ⚠️
cmd/util/cmd/execution-state-extract/cmd.go 62.50% 2 Missing and 1 partial ⚠️
...execution-state-extract/execution_state_extract.go 66.66% 1 Missing ⚠️
Additional details and impacted files
@@                    Coverage Diff                     @@
##           feature/stable-cadence    #5591      +/-   ##
==========================================================
+ Coverage                   55.91%   57.30%   +1.39%     
==========================================================
  Files                        1049      817     -232     
  Lines                      103170    83085   -20085     
==========================================================
- Hits                        57686    47614   -10072     
+ Misses                      41096    31726    -9370     
+ Partials                     4388     3745     -643     
Flag Coverage Δ
unittests 57.30% <65.04%> (+1.39%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@fxamacker fxamacker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! This feature can be very useful for troubleshooting.

I left some comments. The before-migration storage health check needs to happen before migration.MigrateAccount().

cmd/util/ledger/migrations/cadence_values_migration.go Outdated Show resolved Hide resolved
cmd/util/ledger/migrations/cadence.go Outdated Show resolved Hide resolved
cmd/util/ledger/migrations/cadence.go Outdated Show resolved Hide resolved
@turbolent turbolent requested review from fxamacker and a team March 27, 2024 17:49
turbolent and others added 3 commits March 27, 2024 10:50
Copy link
Member

@fxamacker fxamacker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍

Comment on lines +286 to +289
accountBasedMigration := migrationConstructor(
// Only check storage health before the first migration
checkStorageHealthBeforeMigration && index == 0,
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice ! 👍

@turbolent turbolent merged commit e6dd261 into feature/stable-cadence Mar 27, 2024
55 checks passed
@turbolent turbolent deleted the bastian/check-health-before-migration branch March 27, 2024 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants